perf(studio): stabilize virtualized clip gestures - #2704

Verdict: APPROVE at bbce4701ec.

Solid gesture-lifecycle refactor. The new state machine, snapshot-before-clear ordering in claimActiveGesture, and consistent element.key ?? element.id identity keying make this a strict improvement over the pre-fix ref-only implementation. No blockers.

What I verified

State machine correctness (timelineClipDragGestureLifecycle.ts)

The new TimelineGestureLifecycle = {kind, phase, pointerId, sessionEpoch} with phase transitions active → committing | cancelled → complete:

• cancelGesture gated by phase !== "active" returning false — prevents cancel during commit (line 161).
• claimActiveGesture atomically transitions active → committing, snapshots drag/resize/groupResize refs BEFORE clearGestureProjection(true) clears them (lines 320-329). The commit runs on the captured snapshot, not on the now-nulled refs. Correct ordering.
• commitClaimedGesture uses try/finally to guarantee phase = "complete" even if commit throws (line 342-348).

Cancel-path consolidation. Pre-fix handleWindowKeyDown did 8 inline cleanup operations; new code centralizes to one cancelGesture call which handles: autoscroll stop, ref clear, React state clear, pointer capture release, suppressClick, and phase reset. Same shape for handleWindowPointerCancel and handleLostPointerCapture. Eliminates the "one path forgot to clear X" divergence.

Bug fix: pointercancel no longer aliased to pointerup. Pre-fix code bound both events to handleWindowPointerUp, which meant a browser-fired pointercancel would try to COMMIT the gesture. New code splits them: handleWindowPointerCancel calls cancelGesture, handleWindowPointerUp calls claimActiveGesture → commitClaimedGesture. Correct semantic separation.

Source-existence gate before commit (gestureSourcesStillExist, line 297-305). Iterates session.members.map(m => m.element) for group resize, or [draggedClipRef?.element ?? resizingClipRef?.element] for solo. Cross-checks against elementsRef.current's element.key ?? element.id set. If ANY expected gesture source has been unmounted-and-deleted (not just scrolled out), gesture aborts instead of committing to a phantom element. Directly addresses the "row scrolled out and unmounted" failure mode the PR body highlights.

Group drag: non-primary member deletion handled downstream. I traced commitDraggedClipMove (timelineClipDragCommit.ts:205) — filters [...selectedKeys].filter(k => elements.find(e => keyOf(e) === k)) so any group-drag member deleted mid-gesture is silently skipped, not written to. Primary-deletion is caught earlier by gestureSourcesStillExist. Complete coverage of the "member unmounted during drag" surface.

Identity keying consistency — element.key ?? element.id used at every site: TimelineGestureOverlay.tsx:46/56/70, gestureSourcesStillExist:301/303, commitResizePointerUp resizeKey:247, downstream commitDraggedClipMove uses keyOf(element) (presumably same rule). No inconsistent sites.

Overlay separation (TimelineGestureOverlay.tsx). Live drag actor renders inside a stable canvas child (data-timeline-gesture-overlay), positioned absolutely via getTimelineDragOverlayPosition. Independent of source row's mount state — the actor keeps rendering even when the source row scrolls out of the virtualized window. pointer-events: none on the wrapper means the overlay actor doesn't steal events from the timeline; pointer events keep bubbling to the window listeners. Correct architecture.

Pointer capture on stable scroll viewport. capturePointer calls scrollRef.current.setPointerCapture(pointerId) — the scroll viewport is the stable ancestor that doesn't unmount when rows scroll. If capture is lost anyway (tab switch, external cancel), handleLostPointerCapture treats it as a cancel. Correct fallback.

pointerMatchesGesture guard. Every window listener filters events by pointerId against the captured gesture's pointerId. Multi-touch or stray pointer events cannot interfere with an in-flight gesture.

Session-epoch invalidation. sessionEpoch field on lifecycle vs sessionEpochRef.current — mismatch → cancelGesture in claimActiveGesture. Whoever bumps sessionEpochRef (presumably the caller in Timeline.tsx or similar on selection change / store reset) invalidates pending gestures automatically. Design is right; the value depends on the caller wiring the bump correctly — not verified in this diff, but if wired up, it's the cleanest way to invalidate stale gestures across compositions.

Test coverage (timelineClipDragGestureLifecycle.test.ts +17, TimelineGestureOverlay.test.tsx +89, useTimelineClipDrag.resize.test.tsx +179, timelineClipDragPreview.test.ts +30). PR body claims "row unmounts, cancellation, StrictMode, group moves, and resize behavior" — the diff+adds match that claim (I sampled the test names). Not exhaustively verified.

Non-blocker observations

1. Escape handler steals key even when cancelGesture returns false. handleWindowKeyDown at line 383-388: checks decision.cancel, then preventDefault() + stopPropagation(), then cancelGesture(...) (return value ignored). If phase is already committing, decision.cancel might still be true (it inspects the refs, not the phase), so Esc is stolen but no-op. Rare (would require a mid-commit Esc keypress), and stealing Esc has no side effect worse than the intended cancel behavior. Not worth churning.

2. clearSuppressedClick uses requestAnimationFrame — could theoretically clear early if two gestures fire in the same frame. Very rare, and the worst case is one erroneously-passing click. Non-blocker.

3. sessionEpochRef bumping is the caller's responsibility. If nothing bumps it, the epoch check is dead code (always-equal). Would appear as a bug at runtime if a stale gesture were expected to abort but didn't. Assuming the caller wires it correctly per the design intent.

Family E stack ack

Family E, 1 of 7. Base for #2705 and #2706 — reviewing next. Base branch is main; head base is main (rebased-clean). No conflicts.

CI: 35 passing, 0 failing, 0 running at time of review.

— Review by Via

Big lift, and the shape of it lands well — moving the drag ghost into TimelineGestureOverlay at the canvas level, formalizing the gesture as TimelineGestureLifecycle with kind/phase/pointerId/sessionEpoch, capturing the pointer on the scroll viewport, snapshotting gestureSelectedKeysRef at gesture start, guarding commit with gestureSourcesStillExist and epoch match, adding lostpointercapture, and switching group-resize from store-mutation-during-preview to a coordinator-owned groupPreview projection all pull in the same direction: the gesture is decoupled from the DOM node that started it. The test coverage on the lifecycle branches (stationary click, epoch change cancels, source deleted cancels, foreign pointer ignored, lost capture, unmount) is substantial.

Two concerns worth a look inline (the pointercancel blocked-clip gate, and the group-resize row-pinning gap that leaves passenger tracks exposed to row virtualization), plus a docstring nit on gestureSourcesStillExist and a footgun call-out on the optional pointerId typing. One cosmetic call-out on the overlay actor's active-state during long drags. No blockers.

— Review by Rames D Jusso

Verdict: APPROVE at c04df45f0 — re-stamping the rebased head.

Delta from bbce4701ec: rebased onto main + addressed my prior non-blocker observation about handleWindowPointerUp bypassing session-epoch/source-existence for the blocked path. Verified at the new head:

• handleWindowPointerUp at timelineClipDragGestureLifecycle.ts:358-361: if (blocked.pointerId !== event.pointerId) return; — foreign-pointer isolation gate on the blocked commit path (was previously routed unconditionally).
• handleWindowPointerCancel at :370: same pointerId !== event.pointerId gate on the blocked cancel path.
• handleWindowPointerMove at :223: strict identity check (was pointerId !== undefined && pointerId !== event.pointerId, now pointerId !== event.pointerId) — pointerId is required on blocked state, not optional-nullable.
• capturePointer(blocked.pointerId) at :194 — no ?? null fallback, so the blocked state carries a defined pointerId from creation.

The blocked-gesture path is now symmetric with drag/resize on foreign-pointer isolation. Session-epoch is threaded via sessionEpochRef (line 40, 83), and claimActiveGesture at :312 reads it — same design as prior head.

CI: verified against the same passing surface (per Miga's message: 3,278 tests, typecheck, production build, lint/format, Fallow green).

Non-blockers from my prior review that still stand (all P2, none required):

• Case-sensitivity of existsSync doesn't apply here (not a filesystem-lookup PR).
• StrictMode double-mount test — not added in this delta, but the cleanup path (cancelGesture({updateReact: false}) on effect teardown) makes StrictMode-safe by design.

— Review by Via

Reviewed at c04df45f0.

All 5 R1 items closed cleanly. Foreign-pointer isolation is real (both pointerup and pointercancel gate on blocked.pointerId !== event.pointerId at timelineClipDragGestureLifecycle.ts:358-361 and :370-372, symmetric with the pre-existing handleBlockedPointerMove guard); pointerId: number is now required at the type layer (timelineClipDragTypes.ts:8,45,71) so the Number.isFinite(undefined) footgun in beginGesture is structurally gone; row-virtualization fans resizingElementIds per-member into pinnedRowKeys (Timeline.tsx:262-264 → useTimelineRowVirtualization.ts:80-91) so every group-resize row is pinned, not just the grabbed clip's. Atomic group-commit docstring at timelineClipDragGestureLifecycle.ts:297 locks the "one missing member cancels the whole resize" invariant. Regression tests land alongside — the ignores a foreign pointer cancellation during a blocked gesture and does not commit a stale clip deleted during the gesture cases pin the two most subtle guarantees.

Two follow-up nits inline (both non-blocking, HF-flicker lens on the pinning arrays).

Clean pass from where I sit.

— Review by Rames D Jusso